Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix scatter display (alpha/MarkerSize) #58

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Conversation

azabicki
Copy link

Hi Bastian,

I hope it's ok to create a pull request directly without opening any issues. But I only fixed two minor bugs when plotting the two-sided violins.

First, the alpha was set =1 only for the first (right) violin. And secondly, specifying MarkerSize also affected the first (right) violin only.

And the third change in line 237 is just to keep coding-style consistent.

Best,
Adam

@bastibe
Copy link
Owner

bastibe commented Nov 19, 2022

Looks good to me, thank you for the contribution!

However, I don't have access to Matlab any longer, so I can't test it. Let's see if @mikelgg93 wants to have a look as well.

@mikelgg93
Copy link
Collaborator

I would love to test it, but unfortunately, I no longer have access to Matlab either.
One thing before merging, what happens to the median dot size? If I recall it properly, the marker size argument was also used for that, and I did not change it.
Would you mind running the test script and posting the resulting figure here? If the results are okay, I do not have anything against it.
Thanks for contributing!

@azabicki
Copy link
Author

azabicki commented Dec 6, 2022

Hi there,
thanks for your responses!

Sure, here are two figures showing the plots of the test script before and after the commit.

Before:
before

After:
after

And you are correct @mikelgg93, the MarkerSize property affects both, the data dots and the median indicator. I quickly added a new property MedianMarkerSize, so one could set the size of both, the data and the median dots, independently. Defaults are 24 for data dots, and 36 for the median (as previous).

Example from the test script:
violinplot({thisData,repmat(thisData(:,5),1,7)}, catnames_labels, 'ViolinColor', {C,C(5,:)}, 'ViolinAlpha', {0.3 0.3}, 'ShowMean', true, 'MarkerSize', 8, 'MedianMarkerSIze', 80);
add_MedianMarkerSize

I guess I cannot add that commit to this pull request? should I open another one?

@bastibe bastibe merged commit c1545d4 into bastibe:master Dec 7, 2022
@bastibe
Copy link
Owner

bastibe commented Dec 7, 2022

Thank you for your contribution!

If you want to make some more changes, feel free to open another pull request.

@azabicki
Copy link
Author

azabicki commented Dec 7, 2022

Hi there,

ah ok, somehow the "MedianMarkerSize" commit was added to this pull request yesterday, without me being aware of it... don't understand why, because I only pushed it to my personal forked repository.

But then I don't have anything to pull anymore.

@bastibe
Copy link
Owner

bastibe commented Dec 7, 2022

Great!

But that's exactly how pull requests work: You publish a branch in your fork to the main repo. Any change you make to your branch are automatically part of the pull request. So everything worked out!

@azabicki
Copy link
Author

azabicki commented Dec 7, 2022

thanks for the clarification! so I guess changes I commit to my fork which are not supposed to be merged into your repo, should be done in a separate branch.

@bastibe
Copy link
Owner

bastibe commented Dec 7, 2022

exactly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants